Fix passing empty masterkey #585

mmachatschek · 2023-10-11T11:45:54Z

Pull Request

Related issue

Fixes #584

What does this PR do?

Fix the issue by also checking if the provided masterkey is an empty string

PR checklist

Please check if your PR fulfills the following requirements:

Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
Have you read the contributing guidelines?
Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

tests/Http/ClientTest.php

mmachatschek · 2023-10-11T11:48:23Z

@curquiza I didn't revert to the previous behaviour, instead I opted for an check if the user provided an empty string. Empty string can not be passed via authorization anyway so I think this should be safe

Co-authored-by: Tomas Norkūnas <[email protected]>

norkunas

On new major release I'd leave as is documenting that non-empty-string or null are accepted, so maybe a note for future could be added

mmachatschek · 2023-10-11T11:51:58Z

tests/Http/ClientTest.php

+                if ('Authorization' === $name) {
+                    $this->assertSame('Bearer ', $value);
+                }


this actually tests the wrong thing. I'll update the test now

yeah, should be if ('Authorization' === $name) { self::fail(); } :)

@norkunas thanks for the suggestion, didn't see the comment while providing a fix 😅 didn't write PHP tests in a while.

Can you check the latest commit?

dd2d9db

looks good :)

on the second thought, as there is $this->once(), the else case will never be reached I think.
so this would be enough:

$requestStub->expects($this->once()) ->method('withAddedHeader') ->willReturnCallback(function ($name, $value) use ($requestStub) { $this->assertSame('User-Agent', $name); $this->assertSame(Meilisearch::qualifiedVersion(), $value);

mmachatschek · 2023-10-11T12:06:05Z

On new major release I'd leave as is documenting that non-empty-string or null are accepted, so maybe a note for future could be added

@norkunas I don't quite understand. What do you exactly mean?

norkunas · 2023-10-11T14:00:45Z

On new major release I'd leave as is documenting that non-empty-string or null are accepted, so maybe a note for future could be added

@norkunas I don't quite understand. What do you exactly mean?

I mean to add a note about #514 (comment) to not forget it in future :)

tests/Http/ClientTest.php

Co-authored-by: Tomas Norkūnas <[email protected]>

brunoocasali

LGTM!

norkunas · 2023-10-25T03:55:45Z

Maybe we should add deprecation?

curquiza · 2023-10-25T07:54:23Z

bors try

meili-bors · 2023-10-25T07:55:11Z

try

Build failed:

phpstan-tests

curquiza · 2023-10-25T08:01:51Z

@mmachatschek @norkunas I think the tests are still failing 😕

mmachatschek · 2023-10-25T08:26:57Z

@curquiza @brunoocasali I pushed the fix. lets retry with bors

norkunas · 2023-10-25T08:29:21Z

Strange to have two Client classes :)

norkunas · 2023-10-25T08:31:32Z

bors merge

585: Fix passing empty masterkey r=norkunas a=mmachatschek # Pull Request ## Related issue Fixes #584 ## What does this PR do? - Fix the issue by also checking if the provided masterkey is an empty string ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Markus Machatschek <[email protected]> Co-authored-by: Bruno Casali <[email protected]>

meili-bors · 2023-10-25T08:35:58Z

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

{"message":"At least 1 approving review is required by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

norkunas · 2023-10-25T08:38:17Z

bors merge

meili-bors · 2023-10-25T08:42:41Z

Build succeeded:

Fix passing empty masterkey

85848bf

mmachatschek requested review from norkunas, brunoocasali and curquiza October 11, 2023 11:46

curquiza added the bug Something isn't working label Oct 11, 2023

norkunas reviewed Oct 11, 2023

View reviewed changes

tests/Http/ClientTest.php Outdated Show resolved Hide resolved

Update tests/Http/ClientTest.php

3f60c81

Co-authored-by: Tomas Norkūnas <[email protected]>

mmachatschek marked this pull request as draft October 11, 2023 11:49

norkunas previously approved these changes Oct 11, 2023

View reviewed changes

mmachatschek commented Oct 11, 2023

View reviewed changes

fix test

dd2d9db

mmachatschek dismissed norkunas’s stale review via dd2d9db October 11, 2023 12:03

mmachatschek marked this pull request as ready for review October 11, 2023 12:06

Update ClientTest.php

8d3cfff

norkunas reviewed Oct 19, 2023

View reviewed changes

tests/Http/ClientTest.php Show resolved Hide resolved

Update tests/Http/ClientTest.php

d904b10

Co-authored-by: Tomas Norkūnas <[email protected]>

brunoocasali previously approved these changes Oct 24, 2023

View reviewed changes

meili-bors bot added a commit that referenced this pull request Oct 25, 2023

Try #585:

bce49e8

Fix test

0646c63

mmachatschek dismissed brunoocasali’s stale review via 0646c63 October 25, 2023 08:26

mmachatschek mentioned this pull request Oct 25, 2023

Issue when upgrading from < 1.3 to >= 1.3 with api keys set to empty string #584

Closed

norkunas approved these changes Oct 25, 2023

View reviewed changes

mmachatschek merged commit 048eb90 into main Oct 25, 2023
9 checks passed

mmachatschek deleted the issue/584-issue-when-upgrading-from--13-to--13 branch October 25, 2023 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix passing empty masterkey #585

Fix passing empty masterkey #585

mmachatschek commented Oct 11, 2023 •

edited

Loading

mmachatschek commented Oct 11, 2023

norkunas left a comment

mmachatschek Oct 11, 2023

norkunas Oct 11, 2023 •

edited

Loading

mmachatschek Oct 11, 2023

norkunas Oct 11, 2023

norkunas Oct 11, 2023

mmachatschek commented Oct 11, 2023

norkunas commented Oct 11, 2023

brunoocasali left a comment

norkunas commented Oct 25, 2023

curquiza commented Oct 25, 2023

meili-bors bot commented Oct 25, 2023

curquiza commented Oct 25, 2023

mmachatschek commented Oct 25, 2023

norkunas commented Oct 25, 2023

norkunas commented Oct 25, 2023

meili-bors bot commented Oct 25, 2023

norkunas commented Oct 25, 2023

meili-bors bot commented Oct 25, 2023

Fix passing empty masterkey #585

Fix passing empty masterkey #585

Conversation

mmachatschek commented Oct 11, 2023 • edited Loading

Pull Request

Related issue

What does this PR do?

PR checklist

mmachatschek commented Oct 11, 2023

norkunas left a comment

Choose a reason for hiding this comment

mmachatschek Oct 11, 2023

Choose a reason for hiding this comment

norkunas Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

mmachatschek Oct 11, 2023

Choose a reason for hiding this comment

norkunas Oct 11, 2023

Choose a reason for hiding this comment

norkunas Oct 11, 2023

Choose a reason for hiding this comment

mmachatschek commented Oct 11, 2023

norkunas commented Oct 11, 2023

brunoocasali left a comment

Choose a reason for hiding this comment

norkunas commented Oct 25, 2023

curquiza commented Oct 25, 2023

meili-bors bot commented Oct 25, 2023

try

curquiza commented Oct 25, 2023

mmachatschek commented Oct 25, 2023

norkunas commented Oct 25, 2023

norkunas commented Oct 25, 2023

meili-bors bot commented Oct 25, 2023

norkunas commented Oct 25, 2023

meili-bors bot commented Oct 25, 2023

mmachatschek commented Oct 11, 2023 •

edited

Loading

norkunas Oct 11, 2023 •

edited

Loading